Skip to content

Conversation

@parkourben99
Copy link
Contributor

@parkourben99 parkourben99 commented Nov 17, 2024

Fixes #173

Support sub query limits. Now that the Eloquent Eager Limit package is native in Larave 11l.

example usage:

User::with([
    'posts' => fn ($query) => $query->limit(3)
])->paginate();

@parkourben99 parkourben99 marked this pull request as draft November 17, 2024 22:57
@parkourben99 parkourben99 marked this pull request as ready for review November 25, 2024 12:34
@topclaudy
Copy link
Owner

Hi @parkourben99 Thank you for the PR! This package is designed to support Laravel versions prior to 11. However, the Laravel version constraint in your composer.json file would restrict compatibility with those versions.

@parkourben99
Copy link
Contributor Author

parkourben99 commented Jan 10, 2025

Hi @parkourben99 Thank you for the PR! This package is designed to support Laravel versions prior to 11. However, the Laravel version constraint in your composer.json file would restrict compatibility with those versions.

I'm not sure these changes will work with previous versions of laravel, due to the recent change in laravel to support limits.

Do the tests run on previous versions?

@mpyw
Copy link
Contributor

mpyw commented Mar 4, 2025

@parkourben99 @parallels999 Any updates? I'm going to abandon https://github.com/mpyw/compoships-eager-limit. I'm looking forward to this PR get merged!

@parkourben99
Copy link
Contributor Author

parkourben99 commented Mar 4, 2025

@parkourben99 @parallels999 Any updates? I'm going to abandon https://github.com/mpyw/compoships-eager-limit. I'm looking forward to this PR get merged!

Apologies, I have updated this PR.

@mpyw
Copy link
Contributor

mpyw commented Mar 5, 2025

@topclaudy @parallels999 We'll appreciate if you review this in your free time 😃

@parallels999
Copy link

I am not the maintainer of this package, I don't know why you ask me for a review, At most I avoid being left without support in my old apps

@parkourben99
Copy link
Contributor Author

I am not the maintainer of this package, I don't know why you ask me for a review, At most I avoid being left without support in my old apps

Hi @parallels999 I suspect you were tagged as you reviewed this to being with.

@chrispappas
Copy link
Contributor

@parkourben99 I just submitted a PR into your PR 🤯 parkourben99#1

Your changes are not backwards-compatible with older PHP versions, GH Actions test matrix was failing, so I updated the code to use older switch and function closures. Tests are passing now on my fork of your fork.

Fix failing tests, BC with older versions of PHP
@parkourben99
Copy link
Contributor Author

@parkourben99 I just submitted a PR into your PR 🤯 parkourben99#1

Your changes are not backwards-compatible with older PHP versions, GH Actions test matrix was failing, so I updated the code to use older switch and function closures. Tests are passing now on my fork of your fork.

Thanks @chrispappas not sure why dead versions of PHP are still being supported

@chrispappas
Copy link
Contributor

@parkourben99 the linting issue failing the CI build was introduced in my PR (sorry eh 🍁), can you fix please? I'd love to get this PR landed so I can switch back to using the main repo for compoships, and not my personal fork

@chrispappas
Copy link
Contributor

@topclaudy just a follow-up ping on this PR for review, when you have time. I have been using my forked branch with this change in prod for the last few weeks and it works as expected.

@topclaudy
Copy link
Owner

@parkourben99 Can you please fix the failing tests?

@parkourben99
Copy link
Contributor Author

@parkourben99 Can you please fix the failing tests?

Looks like the failing test are due to backed enums. I have merged master into this PR.

@parallels999 I've also added support for mariadb

@topclaudy
Copy link
Owner

@parkourben99 Thanks for the update. The tests are still failing tough. Can you take a look when you have a moment?

@topclaudy
Copy link
Owner

@parkourben99 The tests are green on master 41e7979

@parkourben99
Copy link
Contributor Author

Hi @topclaudy apologies I forgot to push my code, tests should now be all working.

@topclaudy topclaudy merged commit e07b378 into topclaudy:master Oct 3, 2025
33 checks passed
@topclaudy
Copy link
Owner

Thanks @parkourben99 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integration with Eloquent Eager Limit

6 participants